Refactor and test cache #2220
Refactor and test cache #2220
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
This comment has been minimized.
This comment has been minimized.
| @@ -135,7 +131,7 @@ async function setItem( | |||
| // cache-control is still necessary for mini-oxygen | |||
| response.headers.set('cache-control', paddedCacheControlString); | |||
| response.headers.set('real-cache-control', cacheControlString); | |||
| response.headers.set('cache-put-date', new Date().toUTCString()); | |||
| response.headers.set('cache-put-date', String(Date.now())); | |||
There was a problem hiding this comment.
@wizardlyhel Do you remember why this was using toUTCString? It introduces a rounding error (~500ms as mentioned in the note above. Instead, .toISOString() could fix the error but we can also just use the timestamp directly I think?
There was a problem hiding this comment.
urk ... rounding error of ~500ms .. that sucks and how the hell you figure that out?!
I think I was thinking to make the SWR cache calculation to not have timezone differences and this is heavily depends on how the workers and cache data centres are distributed. So .. if we have
- worker 1 in UTC + 7
- worker 2 in UTC + 6
and data centre is at UTC + 7 and both workers are accessing the same data centre, then setting a cache entry on the same key would produce different cache entry duration calculation if they use their local server timestamp
There was a problem hiding this comment.
I understand the reasoning about the different timezone with toUTCString but that function does not return milliseconds so it's impossible to parse it into something accurate (half second rounding error). On the other hand, toISOString contains milliseconds:
toUTCString:Mon, 10 Jun 2024 02:03:08 GMTtoISOString:2024-06-10T02:03:08.920Z
They are both always based on UTC.
In any case, I think a simple timestamp based on UTC is also fine and even simpler, right?
There was a problem hiding this comment.
Oh! I didn't know that UTCString doesn't supply the milliseconds. Yea, let's use ISOString
There was a problem hiding this comment.
But Date.now() also considers UTC and is simpler conceptually to store milliseconds than a date. Let's use that better, no?
There was a problem hiding this comment.
Agreed, I think Date.now() is simpler
| const VALUE = 'my-value'; | ||
| const actionFn = vi.fn(() => VALUE); | ||
| const waitUntil = vi.fn(() => {}); | ||
| let cache: InMemoryCache; |
There was a problem hiding this comment.
For now this test uses our InMemoryCache implementation. We could change this to eventually use real MiniOxygen cache directly but I think it's good for now.
There was a problem hiding this comment.
This is awesome! Thank you for doing this. One nitpicky thing, but maybe an extra test that just validates cache expiration?
There was a problem hiding this comment.
There's already one in this file, inside the SWR test. It first checks SWR, then it checks what happens when the whole thing expires 👍
There was a problem hiding this comment.
The only change to this code is renaming a couple of private types. Everything else is extracted from src/cache/fetch.ts
This PR should be checked commit by commit:
fetchWithServerCachein a different file.createWithCachefrom root tocachedir.createWithCache.